Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

corrected upfront gas deduction for type-2 transactions #1849

Closed
wants to merge 5 commits into from

Conversation

rachit77
Copy link
Contributor

@rachit77 rachit77 commented Aug 24, 2023

Description

These changes will be implemented as hard fork.

This PR resolves the bug reported in EVM-803 and EVM-804

EVM-803: As of now upfront amount deducted for dynamic transaction was gas*maxGasFeeCap which is wrong.
upfront amount should be gas*gasPrice and for dynamic transaction gasPrice = Min((gasTipCap + baseFee), gasFeeCap)

EVM-804: Txpool didn't handle the legacy transactions properly when London fork is enabled.

  1. Underpriced legacy transaction were not discarded in txpool.
  2. Coinbase tip was wrong in case of Legacy transactions.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

Executed a transaction and compared the initial and final account balance of burn contract, validator, transaction sender and transaction receiver

@rachit77 rachit77 added the bug fix Functionality that fixes a bug label Aug 24, 2023
@rachit77 rachit77 self-assigned this Aug 24, 2023
@rachit77 rachit77 added don't merge Please don't merge this functionality temporarily breaking change Functionality that contains breaking changes labels Aug 24, 2023
@@ -450,7 +450,7 @@ func (t *Transition) subGasLimitPrice(msg *types.Transaction) error {
factor := new(big.Int)
if msg.GasFeeCap != nil && msg.GasFeeCap.BitLen() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi team, just trying to better understand the implementation of the executor.go and I would like to ask a question.

Why check msg.GasFeeCap instead of msg.Type? The txpool.validateTx enforces that all DynamicFee transactions (i.e., msg.Type == DynamicFeeTx) have a gasFeeCap and a gasTipCap, and gasFeeCap >= baseFee. Is there a better guarantee in checking the gasFeeCap here? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no strict reason why we should go this way rather than checking tx type. The idea was to be more explicit and avoid potential nil pointer panic issue there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be consistent through code-base. I vote for type check everywhere. If we have dynamic tx without those fields than we have much bigger problem

@@ -450,7 +450,7 @@ func (t *Transition) subGasLimitPrice(msg *types.Transaction) error {
factor := new(big.Int)
if msg.GasFeeCap != nil && msg.GasFeeCap.BitLen() > 0 {
// Apply EIP-1559 tx cost calculation factor
factor = factor.Set(msg.GasFeeCap)
factor = factor.Set(msg.GetGasPrice(t.ctx.BaseFee.Uint64()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesterday I sent this transaction to Polygon Edge and it was mined:

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "nonce": "0x0",
    "gasPrice": "0x6b",
    "maxPriorityFeePerGas": "0x64",
    "maxFeePerGas": "0x19a45965",
    "gas": "0x5208",
    "to": "0x1064c5ddD8635bA3D8FEe5cd8DEf8d502721EcEa",
    "value": "0x3b9aca00",
    "input": "0x",
    "v": "0x1",
    "r": "0x88e8f015ad334bc048c60b5561791a31ced4f8e87237eea00794c080eb13db5c",
    "s": "0x30758b64ea1472a298b0f8f85d0f9fd15c1e2b7f9eae26744dc4a597bb9f52d8",
    "hash": "0x209cefcd3bf2c6713195e4732575824827e13b7977d48002e5f039fdf15838bc",
    "from": "0xB23650A2b25aB51590e3b4e66bFc4AE426Eb1cA5",
    "blockHash": "0xe7bfc47dac8bd7c1fe1390cc4d55fbf1bfcc7d158a457326d425cf4ffd29541f",
    "blockNumber": "0x139",
    "transactionIndex": "0x0",
    "chainId": "0x343c",
    "type": "0x2"
  }
}

Notice that it has all gasPrice, maxFeePerGas and maxPriorityFeePerGas. Also, gasPrice is a lot lower than maxFeePerGas. baseFee for the block was 0x7, as you can see here:

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "parentHash": "0xa3b9b3c72a5bcfe37e4259026033f0ead7e4d950160d509c177c74426b6f4513",
    "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
    "miner": "0x46bc0fc20e0c71bd74bef78095fb68be8d0f9f58",
    "stateRoot": "0xaf4c8dcc798a862af50dadd16718ee80e56ba7d48e2d6ca528e7f672e9d7c463",
    "transactionsRoot": "0x9d248b1717270cb57407026f4e5bfa3ea060d9e4768b056a8e40ede234532dec",
    "receiptsRoot": "0x056b23fbba480696b65fe5a59b8f2148a1299103c4f57df839233af2cf4ca2d2",
    "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
    "difficulty": "0x1",
    "totalDifficulty": "0x1",
    "size": "0x345",
    "number": "0x139",
    "gasLimit": "0xb2d05e00",
    "gasUsed": "0x5208",
    "timestamp": "0x64e68fa1",
    "extraData": "0x0000000000000000000000000000000000000000000000000000000000000000f8b0c0f843b84029d32f595b6b76fa23111138ab770da9698418f6caba9a3427b08e3a38d44ee003cb04b88acfea9c0a85cda63b156979daaa3f28a534ab90937c494cca1d7cdf07c28080f8658002a00166805ec67e416c4d360ecea2d51bab58cbfbd1dab4b612180dfc5aabf48aafa00166805ec67e416c4d360ecea2d51bab58cbfbd1dab4b612180dfc5aabf48aafa00000000000000000000000000000000000000000000000000000000000000000",
    "mixHash": "0xadce6e5230abe012342a44e4e9b6d05997d6f015387ae0e59be924afc7ec70c1",
    "nonce": "0x0000000000000000",
    "hash": "0xe7bfc47dac8bd7c1fe1390cc4d55fbf1bfcc7d158a457326d425cf4ffd29541f",
    "transactions": [
      "0x209cefcd3bf2c6713195e4732575824827e13b7977d48002e5f039fdf15838bc"
    ],
    "uncles": [],
    "baseFeePerGas": "0x7"
  }
}

Does the implementation of GetGasPrice(baseFee uint64) mean that I can force a lower price for DynamicFeeTx because gasPrice will be picked over baseFee + gasTipCap / gasFeeCap? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jp-imx can you please share the transaction object which you used as well as the tool/library employed for the purpose of transaction signing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rachit77 I used cast from foundry. I believe I ran this command:

cast send <RECIPIENT_ADDRESS> --value 1gwei --rpc-url http://127.0.0.1:40002/ --gas-price 430201189wei  --priority-gas-price=1wei --private-key <PRIVATE_KEY> --nonce 11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jp-imx Answering your original question:

You signed a type-2(dynamic tx) with cast with maxFeePerGas = 430201189 wei and maxPriorityFee= 100 wei, the transaction was included in block when baseFee was 7 wei.
Effective gas price that will be charged according to EIP-1559 specs, min( maxFeePerGas, baseFee+maxPriorityFee).
With the above formula effective gasPrice for your above transaction should be 107 wei and it is the same.

Notice that it has all gasPrice, maxFeePerGas and maxPriorityFeePerGas. Also, gasPrice is a lot lower than maxFeePerGas. baseFee for the block was 0x7, as you can see here:

The receipt of dynamic tx has all three fields, maxFeePerGas and maxPriorityFeePerGas will be there since they are provided with the transaction. gasPrice is the effective gas price that is deduced and charged according to EIP-1559 specifications.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rachit77, thanks for getting back to me. I agree with your analysis. But taking it one step further, given the implementation of GetGasPrice starts like this:

func (t *Transaction) GetGasPrice(baseFee uint64) *big.Int {
	if t.GasPrice != nil && t.GasPrice.BitLen() > 0 {
		return new(big.Int).Set(t.GasPrice)
	} else if baseFee == 0 {
		return big.NewInt(0)
	}

	//...
}

If I create a custom Tx object that sets gasFeeCap at a very high number but gasPrice to 1, could I underpay for my transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jp-imx

cast send <RECIPIENT_ADDRESS> --value 1gwei --rpc-url http://127.0.0.1:40002/ --gas-price 430201189wei  --priority-gas-price=1wei --private-key <PRIVATE_KEY> --nonce 11

The above transaction will be treated as Type-2 transaction by cast, although you have provided gas-price value in the transaction but signer will treat this as maxFeePerGas.

Foundry Docs here:

--gas-price price
Gas price for the transaction, or max fee per gas for EIP1559 transactions.

When edge client receives this signed transaction, the transaction won't have gasPrice field but rather it will have GasFeeCap(maxFeePerGas) and GasTipCap(maxPriorityFee). So there is no possibility of underpaying for a transaction.

Copy link
Contributor

@jp-imx jp-imx Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rachit77 I don't think using cast is relevant. I can roll custom code for sending a raw transaction and include all three gasPrice, gasFeeCap, and gasTipCap. The issue is mute though because on further inspection I found that rlp_unmarshal.go ignores gasPrice and if all three are included the Tx is rejected with incorrect number of transaction elements, expected 12 but found 13. So the transaction would never reach GetGasPrice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that is right

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GetGasPrice function looks strange to me as well and it seems it would be more natural to check transaction type if DynamicFeeTx should not have a gas price set.

txpool/txpool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, just address pending comments.

rachit77 added a commit to rachit77/polygon-edge that referenced this pull request Sep 3, 2023
@rachit77
Copy link
Contributor Author

rachit77 commented Sep 5, 2023

The changes from this PR are included in 1857

rachit77 added a commit that referenced this pull request Sep 5, 2023
* corrected upfront gas deduction for type-2 transactions

* solve the bug reported in EVM-804

* code optimization

* testCase fix

* added logic

* fix linting error

* refactored code

* consensus should also have baseFee and gasPrice/gasFeeCap check for legacy and dynamic tx respecitively

* linter fix

* cr fix

* don't build executable heap with baseFee

* fixed txpool e2e tests

* fixed e2e migration test

* logic for fork handler

* fix linting

* fix unit test

* added hard fork logic for PR #1849

* fixed TestE2E_TxPool_Transfer_Linear

* added e2e test case for EIP-1559 spec

* fix Test_Transition_checkDynamicFees
@rachit77 rachit77 closed this Sep 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change Functionality that contains breaking changes bug fix Functionality that fixes a bug don't merge Please don't merge this functionality temporarily
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants